Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hakyll.Core.Runtime: use MVar instead of TVar #863

Merged
merged 1 commit into from
Jul 18, 2021
Merged

Hakyll.Core.Runtime: use MVar instead of TVar #863

merged 1 commit into from
Jul 18, 2021

Conversation

vaibhavsagar
Copy link
Contributor

Proposing a slightly different choice of concurrent variable type to #844.

@vaibhavsagar
Copy link
Contributor Author

quick benchmarks on my own site at https://github.com/vaibhavsagar/website, result-new is this PR and result-old is d739fd1:

[nix-shell:~/code/website]$ hyperfine --prepare './result-new/bin/site clean' './result-new/bin/site build' --prepare './result-old/bin/site clean' './result-old/bin/site build'
Benchmark #1: ./result-new/bin/site build
  Time (mean ± σ):      3.335 s ±  0.306 s    [User: 17.169 s, System: 4.343 s]
  Range (min … max):    3.036 s …  4.075 s    10 runs
 
Benchmark #2: ./result-old/bin/site build
  Time (mean ± σ):      3.434 s ±  0.213 s    [User: 17.648 s, System: 4.595 s]
  Range (min … max):    3.106 s …  3.734 s    10 runs
 
Summary
  './result-new/bin/site build' ran
    1.03 ± 0.11 times faster than './result-old/bin/site build'

@vaibhavsagar
Copy link
Contributor Author

Huh, I tried running it again in the other order and it gave me the opposite result:

[nix-shell:~/code/website]$ hyperfine --prepare './result-old/bin/site clean' './result-old/bin/site build' --prepare './result-new/bin/site clean' './result-new/bin/site build'
Benchmark #1: ./result-old/bin/site build
  Time (mean ± σ):      3.447 s ±  0.288 s    [User: 17.842 s, System: 4.563 s]
  Range (min … max):    3.117 s …  4.135 s    10 runs
 
Benchmark #2: ./result-new/bin/site build
  Time (mean ± σ):      3.515 s ±  0.289 s    [User: 18.079 s, System: 4.467 s]
  Range (min … max):    3.096 s …  4.027 s    10 runs
 
Summary
  './result-old/bin/site build' ran
    1.02 ± 0.12 times faster than './result-new/bin/site build'

@Minoru
Copy link
Collaborator

Minoru commented Jul 17, 2021

I still don't know anything about async Haskell, but I read the Control.Concurrent.MVar doc and these changes look okay. So I'll merge this.

Huh, I tried running it again in the other order and it gave me the opposite result:

Your environment is probably noisy. On my machine, MVar consistently out-performs TVar by 3–4%:

$ hyperfine --prepare './debiania-old clean' './debiania-old build +RTS -N4' --prepare './debiania-new clean' './debiania-new build +RTS -N4'
Benchmark #1: ./debiania-old build +RTS -N4
  Time (mean ± σ):     10.344 s ±  0.246 s    [User: 21.688 s, System: 4.336 s]
  Range (min … max):    9.992 s … 10.654 s    10 runs

Benchmark #2: ./debiania-new build +RTS -N4
  Time (mean ± σ):     10.077 s ±  0.329 s    [User: 21.009 s, System: 4.331 s]
  Range (min … max):    9.502 s … 10.674 s    10 runs

Summary
  './debiania-new build +RTS -N4' ran
    1.03 ± 0.04 times faster than './debiania-old build +RTS -N4'
$ hyperfine --prepare './debiania-new clean' './debiania-new build +RTS -N4' --prepare './debiania-old clean' './debiania-old build +RTS -N4'
Benchmark #1: ./debiania-new build +RTS -N4
  Time (mean ± σ):      9.973 s ±  0.312 s    [User: 20.773 s, System: 4.183 s]
  Range (min … max):    9.432 s … 10.505 s    10 runs

Benchmark #2: ./debiania-old build +RTS -N4
  Time (mean ± σ):     10.332 s ±  0.293 s    [User: 21.484 s, System: 4.463 s]
  Range (min … max):    9.759 s … 10.690 s    10 runs

Summary
  './debiania-new build +RTS -N4' ran
    1.04 ± 0.04 times faster than './debiania-old build +RTS -N4'

I was hoping that MVar might fix #850, but no:

$ hyperfine --parameter-scan threads 1 10 --prepare './debiania-new clean' './debiania-new build +RTS -N{threads}'
Benchmark #1: ./debiania-new build +RTS -N1
  Time (mean ± σ):     14.021 s ±  0.334 s    [User: 13.887 s, System: 0.646 s]
  Range (min … max):   13.369 s … 14.643 s    10 runs

Benchmark #2: ./debiania-new build +RTS -N2
  Time (mean ± σ):     10.909 s ±  0.208 s    [User: 15.330 s, System: 1.907 s]
  Range (min … max):   10.632 s … 11.317 s    10 runs

Benchmark #3: ./debiania-new build +RTS -N3
  Time (mean ± σ):     10.120 s ±  0.251 s    [User: 17.742 s, System: 2.916 s]
  Range (min … max):    9.804 s … 10.511 s    10 runs

Benchmark #4: ./debiania-new build +RTS -N4
  Time (mean ± σ):     10.118 s ±  0.281 s    [User: 21.168 s, System: 4.287 s]
  Range (min … max):    9.481 s … 10.495 s    10 runs

Benchmark #5: ./debiania-new build +RTS -N5
  Time (mean ± σ):     10.636 s ±  0.220 s    [User: 26.352 s, System: 6.382 s]
  Range (min … max):   10.223 s … 10.922 s    10 runs

Benchmark #6: ./debiania-new build +RTS -N6
  Time (mean ± σ):     10.910 s ±  0.298 s    [User: 30.955 s, System: 8.487 s]
  Range (min … max):   10.533 s … 11.466 s    10 runs

Benchmark #7: ./debiania-new build +RTS -N7
  Time (mean ± σ):     11.228 s ±  0.193 s    [User: 36.000 s, System: 10.565 s]
  Range (min … max):   10.925 s … 11.554 s    10 runs

Benchmark #8: ./debiania-new build +RTS -N8
  Time (mean ± σ):     12.273 s ±  0.264 s    [User: 46.391 s, System: 12.316 s]
  Range (min … max):   11.845 s … 12.555 s    10 runs

Benchmark #9: ./debiania-new build +RTS -N9
  Time (mean ± σ):     13.482 s ±  0.183 s    [User: 52.771 s, System: 14.147 s]
  Range (min … max):   13.183 s … 13.845 s    10 runs

Benchmark #10: ./debiania-new build +RTS -N10
  Time (mean ± σ):     14.051 s ±  0.113 s    [User: 56.190 s, System: 14.633 s]
  Range (min … max):   13.815 s … 14.212 s    10 runs

Summary
  './debiania-new build +RTS -N4' ran
    1.00 ± 0.04 times faster than './debiania-new build +RTS -N3'
    1.05 ± 0.04 times faster than './debiania-new build +RTS -N5'
    1.08 ± 0.04 times faster than './debiania-new build +RTS -N2'
    1.08 ± 0.04 times faster than './debiania-new build +RTS -N6'
    1.11 ± 0.04 times faster than './debiania-new build +RTS -N7'
    1.21 ± 0.04 times faster than './debiania-new build +RTS -N8'
    1.33 ± 0.04 times faster than './debiania-new build +RTS -N9'
    1.39 ± 0.05 times faster than './debiania-new build +RTS -N1'
    1.39 ± 0.04 times faster than './debiania-new build +RTS -N10'

@Minoru Minoru merged commit db14392 into jaspervdj:master Jul 18, 2021
@Minoru
Copy link
Collaborator

Minoru commented Jul 18, 2021

Thank you @vaibhavsagar!

@vaibhavsagar
Copy link
Contributor Author

My pleasure!

@vaibhavsagar vaibhavsagar deleted the replace-tvar-with-mvar branch July 18, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants